Skip to content

Swift: Add taint models for the Data class #11345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 24, 2022

Conversation

atorralba
Copy link
Contributor

No description provided.

@atorralba atorralba requested a review from a team as a code owner November 21, 2022 13:45
@github-actions github-actions bot added the Swift label Nov 21, 2022
geoffw0
geoffw0 previously approved these changes Nov 22, 2022
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

I've made a few minor suggestions, none of them should be considered blocking.

let stringTainted = String(data: source3(), encoding: String.Encoding.utf8)

sink(arg: stringClean!)
sink(arg: stringTainted!) // $ MISSING: tainted=100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the .utf8 case earlier) is presumably a job for the String model rather than Data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's why I moved them to the string.swift file (they were previously on data.swift). They ought to be fixed by #11185.

let dataTainted20 = source() as! Data
let pointerTainted20 = UnsafeMutablePointer<UInt8>.allocate(capacity: 0)
dataTainted20.copyBytes(to: pointerTainted20, from: 0..<1)
sink(arg: pointerTainted20) // $ MISSING: tainted=153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any idea what's gone wrong in these two cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really :(

I think it may have to do with the type UnsafeMutablePointer, but I'm surprised such a direct flow doesn't work regardless of the type of the tainted object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we should create an issue to investigate this, though I'm a little reluctant to prioritize it above all the other stuff.

@atorralba atorralba requested a review from a team as a code owner November 23, 2022 11:18
@atorralba atorralba force-pushed the atorralba/swift/data-models branch from 3b0a001 to e2afeeb Compare November 23, 2022 11:21
@atorralba atorralba force-pushed the atorralba/swift/data-models branch from b114dc4 to fc7c66d Compare November 24, 2022 11:39
@atorralba
Copy link
Contributor Author

Rebased to remove the special-cased additional taint step for the Data constructor from #11001.

arg =
any(CallExpr ce | ce.getStaticTarget().(MethodDecl).hasQualifiedName("Data", "init(_:)"))
.getArgument(0)
or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

geoffw0
geoffw0 previously approved these changes Nov 24, 2022
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge I think.

@atorralba atorralba merged commit 1d57663 into github:main Nov 24, 2022
@atorralba atorralba deleted the atorralba/swift/data-models branch November 24, 2022 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants